GH-143948: Explain graphlib's cycle-finding code #143950
Open
+56
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tim-one
added
skip news
stdlib
tim-one
changed the title
picnixz
reviewed
Jan 18, 2026
Comment on lines
+258
to
+259
|
|
||
| # On Finding Cycles |
Member
There was a problem hiding this comment.
Suggested change
| # On Finding Cycles | |
| # On Finding Cycles |
| # There is a (at least one) total order if and only if the graph is | ||
| # acyclic. | ||
| # | ||
| # When it is cyclic, "there's a cycle - somewhere!" isn't very helpful. |
Member
There was a problem hiding this comment.
I would argue that knowing that there is a cycle could still be useful! so
Suggested change
| # When it is cyclic, "there's a cycle - somewhere!" isn't very helpful. | |
| # When it is cyclic, "there's a cycle - somewhere!" is not always helpful. |
| # | ||
| # When it is cyclic, "there's a cycle - somewhere!" isn't very helpful. | ||
| # In theory, it would be most helpful to partition the graph into | ||
| # strongly connected components (SCCs) and display those with more than |
Member
There was a problem hiding this comment.
Others could argue that WCCs are also enough for their cases. Do you want to explain the difference?
| # That's a lot of work, though, and we can get most of the benefit much | ||
| # more easily just by showing a single specific cycle. | ||
| # | ||
| # Finding a cycle is most natural via a breadth first search, which can |
Member
There was a problem hiding this comment.
Suggested change
| # Finding a cycle is most natural via a breadth first search, which can | |
| # Finding a cycle is most natural via a breadth-first search (BFS), which can |
I am not entirely sure for the dash. I would however mention BFS because readers may be familiar with the abbreviation (I am personally more used to the abbrev than the full word... but this is what happens when you use it a lot).
Comment on lines
+306
to
+307
| # Worst case time is linear in the number of nodes plus the number of | ||
| # edges. Worst case memory burden is linear in the number of nodes. |
Member
There was a problem hiding this comment.
Suggested change
| # Worst case time is linear in the number of nodes plus the number of | |
| # edges. Worst case memory burden is linear in the number of nodes. | |
| # If |V| is the number of vertices and |E| is the number of edges, | |
| # worst case time is O(|V| + |E|) and worst case memory burden is O(|V|). |
Strictly speaking this is just the usual complexity of DFS. But I think we can use the same notations as Wikipedia here. Unless you explicitly want to avoid O-notation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
gh-143948: Explain graphlib's cycle-finding code
Long overdue comments explaining what's going on, and why. No code changes, just comments.